feat(rust/sedona-spatial-join) Making SpatialIndex a trait by pwrliang · Pull Request #645 · apache/sedona-db

pwrliang · 2026-02-19T22:39:07Z

This PR splits SpatialIndex into a SpatialIndex trait and a default implementation DefaultSpatialIndex. These changes are for the upcoming GPU-based spatial join.

Kontinuation

The refactoring for SpatialIndex looks good to me. BTW, I believe that SpatialIndexBuilder also need to be a trait.

Kontinuation · 2026-02-20T06:16:42Z

rust/sedona-spatial-join/src/index/spatial_index.rs

-        self.probe_threads_counter.fetch_sub(1, Ordering::Relaxed) == 1
-    }
-
+    fn report_probe_completed(&self) -> bool;


We can do another pass of refactoring to move probe completion markers out of spatial index, this will simplify the interface of SpatialIndex. This does not need to be done in this PR.

paleolimbot · 2026-02-20T15:39:11Z

Thank you for opening!

I'm happy to take a look after the 0.3.0 release branch is created (hopefully Monday!).

paleolimbot

Thank you!

I defer to @Kontinuation for the details (this may need another look as the builder trait was also added). I added a note about retaining documentation with the trait members rather than the implementation because I think they're most useful with the trait.

I'll also run copilot on this because I think it may help here!

rust/sedona-spatial-join/src/index/spatial_index.rs

Copilot

Pull request overview

This PR refactors the spatial-join indexing layer to introduce a SpatialIndex trait with a default implementation (DefaultSpatialIndex), enabling future alternative index backends (e.g., GPU-based) while keeping the current R-tree-based behavior as the default.

Changes:

Split the previous concrete SpatialIndex into a SpatialIndex trait plus DefaultSpatialIndex implementation, and introduced SpatialIndexRef as an Arc<dyn ...> handle.
Split the previous concrete builder into a SpatialIndexBuilder trait plus DefaultSpatialIndexBuilder.
Updated join stream/index-provider plumbing (and tests) to use the trait-object index handle instead of Arc<SpatialIndex>.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
rust/sedona-spatial-join/src/stream.rs	Switches stream state and iterators to store/use `SpatialIndexRef` instead of a concrete index type.
rust/sedona-spatial-join/src/prepare.rs	Updates imports for build metrics after moving them under `spatial_index_builder`.
rust/sedona-spatial-join/src/lib.rs	Adjusts (and currently comments out) public re-exports related to index/build metrics.
rust/sedona-spatial-join/src/index/spatial_index_builder.rs	Introduces `SpatialIndexBuilder` trait and keeps `SpatialJoinBuildMetrics`.
rust/sedona-spatial-join/src/index/spatial_index.rs	Replaces concrete index implementation with `SpatialIndex` trait + `SpatialIndexRef`.
rust/sedona-spatial-join/src/index/partitioned_index_provider.rs	Migrates provider cache/build APIs to return `SpatialIndexRef` and uses `DefaultSpatialIndexBuilder`.
rust/sedona-spatial-join/src/index/default_spatial_index_builder.rs	New default builder implementing `SpatialIndexBuilder` and returning `SpatialIndexRef`.
rust/sedona-spatial-join/src/index/default_spatial_index.rs	New default index implementing `SpatialIndex`, moving prior index logic/tests here.
rust/sedona-spatial-join/src/index/build_side_collector.rs	Switches memory estimation call to `DefaultSpatialIndexBuilder::estimate_extra_memory_usage`.
rust/sedona-spatial-join/src/index.rs	Registers new index modules and re-exports `DefaultSpatialIndexBuilder` (currently as `pub`).

Comments suppressed due to low confidence (2)

rust/sedona-spatial-join/src/index/spatial_index.rs:34

DISTANCE_TOLERANCE is declared pub inside a pub(crate) module under a private index module, so it isn’t effectively public. This may trigger unreachable_pub warnings. Consider changing it to pub(crate) (or re-export it from the crate root if it’s intended as public API).

pub const DISTANCE_TOLERANCE: f64 = 1e-9;

rust/sedona-spatial-join/src/index/spatial_index.rs:99

SpatialIndexRef is a pub type alias, but it lives in a pub(crate) module under a private index module, so it isn’t effectively public and may trigger unreachable_pub warnings. Consider changing it to pub(crate) (internal use) or re-exporting it from lib.rs if external consumers should be able to reference index handles.

pub type SpatialIndexRef = Arc<dyn SpatialIndex + Send + Sync>;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

rust/sedona-spatial-join/src/lib.rs

rust/sedona-spatial-join/src/index.rs

rust/sedona-spatial-join/src/index/spatial_index_builder.rs

rust/sedona-spatial-join/src/index/default_spatial_index_builder.rs

rust/sedona-spatial-join/src/index/default_spatial_index.rs

rust/sedona-spatial-join/src/index/build_side_collector.rs

rust/sedona-spatial-join/src/index/spatial_index_builder.rs

pwrliang · 2026-03-05T18:00:16Z

I think I have addressed the above issues. I will open another PR to simplify the inteface of SpatialIndex, e.g., moving report_probe_completed, need_more_probe_stats out of SpatialIndex

pwrliang force-pushed the spatial-index-trait branch from dcab75a to 085a047 Compare February 19, 2026 22:43

Kontinuation approved these changes Feb 20, 2026

View reviewed changes

Kontinuation reviewed Feb 20, 2026

View reviewed changes

pwrliang changed the title ~~Making SpatialIndex a trait~~ feat(rust/sedona-spatial-join) Making SpatialIndex a trait Feb 20, 2026

paleolimbot reviewed Mar 3, 2026

View reviewed changes

rust/sedona-spatial-join/src/index/spatial_index.rs Outdated Show resolved Hide resolved

rust/sedona-spatial-join/src/index/spatial_index.rs Show resolved Hide resolved

paleolimbot requested a review from Copilot March 3, 2026 04:04

Copilot started reviewing on behalf of paleolimbot March 3, 2026 04:05 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

Kontinuation reviewed Mar 4, 2026

View reviewed changes

rust/sedona-spatial-join/src/index/build_side_collector.rs Outdated Show resolved Hide resolved

rust/sedona-spatial-join/src/index/spatial_index_builder.rs Outdated Show resolved Hide resolved

pwrliang added 4 commits March 5, 2026 10:26

Add SpatialIndex trait

5ef9b26

Fix pre-commit

031d625

Providing a SpatialIndexBuilder trait and DefaultSpatialIndexBuilder

4685f58

fix code review issues

2aaa1d2

pwrliang force-pushed the spatial-index-trait branch from c217bf7 to 2aaa1d2 Compare March 5, 2026 15:30

Bugfix

827fb63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust/sedona-spatial-join) Making SpatialIndex a trait#645

feat(rust/sedona-spatial-join) Making SpatialIndex a trait#645
pwrliang wants to merge 5 commits intoapache:mainfrom
pwrliang:spatial-index-trait

pwrliang commented Feb 19, 2026

Uh oh!

Kontinuation left a comment

Uh oh!

Kontinuation Feb 20, 2026

Uh oh!

paleolimbot commented Feb 20, 2026

Uh oh!

paleolimbot left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pwrliang commented Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

pwrliang commented Feb 19, 2026

Uh oh!

Kontinuation left a comment

Choose a reason for hiding this comment

Uh oh!

Kontinuation Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

paleolimbot commented Feb 20, 2026

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pwrliang commented Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants